Skip to content

Remove bindings to private or c std types #149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 4, 2025

Conversation

ginnyTheCat
Copy link
Collaborator

@ginnyTheCat ginnyTheCat commented Jun 3, 2025

This makes functions and types specified as internal not show up in the bindings. On top it also only exports the needed C std types and makes them opaque.

Before
rustdoc screenshot with more C std types

After
rustdoc screenshot with less C std types

The harfbuzz cc flag fixes an error that occurs on my machine when cross compiling to windows (the harfbuzz-sys crate also has this flag set).

@ginnyTheCat
Copy link
Collaborator Author

It now also removes the va_list functions as all of them can be replaced by ... functions, and va_list seems to behave very differently between architectures and is not really usable in rust yet (this is all just from my understanding and probably wrong).

@messense messense merged commit 4cbf146 into messense:main Jun 4, 2025
14 checks passed
@ginnyTheCat ginnyTheCat deleted the remove-bindings branch June 4, 2025 04:27
@ginnyTheCat
Copy link
Collaborator Author

Sorry for only noticing this only after merging and looking at the main branch CI logs.

The windows build now prints warnings about u128 being not being FFI-safe. This is because fz_context.error.stack[0].buffer is of type jmp_buf which has an alignment of 16 on windows. To keep this alignment bindgen uses u128 inside the opaque type.

This warning is, however, is not releavant for us for many reasons:

  1. we never pass a fz_context over FFI boundaries, only *mut fz_context
  2. this has been fixed in rust 1.86.0 ...
  3. ... and therefore the warning is planned to be removed as well

What I think would reasonable is to either

  • allow(improper_ctypes_definitions) in mupdf-sys. This would silence other improper_ctypes_definitions warning that might be useful though.
  • skip opaque-ing fz_context for now, hiding the types with this PR is more of a "style" and "correctness" thing, so removing it for now would be not a problem.
  • adding fz_context as our own opaque type not generated by bindgen. As we never need to create a fz_context on the rust side, but only have a pointer to it given to us by mupdf, we don't have to worry about it's alignment, and therefore don't have to fill it with u128 to achieve this. This sounds more confident than I am to have gotten the details right, but I think it makes sense.

@messense
Copy link
Owner

messense commented Jun 5, 2025

Feel free to send follow-up PRs and merge when CI passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants